Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314

TsafrirA · 2022-12-20T23:51:37Z

Summary

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To overcome the non-unique nature of the new representation (particularly, when negative amp is allowed), this PR introduces a new subclass ScalableSymbolicPulse which assumes the amp,angle representation, and compares the pulses accordingly.

PR #9257 presented a more general solution for any non-unique representation, but it was debated whether such a general solution is in fact needed.

Details and comments

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To better support several experiments in Qiskit-Experiments, the amp parameter is allowed to take both positive and negative values. This decision creates a non-unique representation for the pulses, where a pi shift of angle and a sign flip of amp have the same effect (the issue obviously also exists for 2*pi shift of angle). For example, the following two pulses

from qiskit.pulse.library import Gaussian
import numpy as np

g1 = Gaussian(duration=100, sigma=50, amp=-1, angle=0)
g2 = Gaussian(duration=100, sigma=50, amp=1, angle=np.pi)

are not the same when equated (because their parameters are different):

g1 == g2 # False

but they represent the same waveform:

np.all(g1.get_waveform().samples == g2.get_waveform().samples) # True

This PR sets to correct this situation, by introducing a new subclass ScalableSymbolicPulse. This class assumes that the pulse envelope has the form amp * exp(1j * angle)*F(t,parameters) where F is some function. When two instances of this class are equated, the complex amplitude amp * exp(1j * angle) is compared, but the individual parameters amp,angle are ignored.

The comparison is being done using numpy.isclose(), with default values of rtol=1.e-5, atol=1.e-8, which I think is sufficient**.

the comparison of the complex amplitude (and the hashing) is carried out after a rounding to 6 decimal places. Considering the expected scale of the complex amplitude, and HW finite precision, this value should be sufficient to distinguish pulses which are really different from one another.

To accommodate the change, QPY format now includes a class_name field, which indicates the correct class when loading the pulse. QPY version was bumped to 6 to reflect this. QPY files of older versions assign only library pulses to the new subclass (even if a custom pulse might fit its purpose).

Returning to the example above, the new PR yields:

g1 == g2 # True

Along the way, the issue of hashing was raised. Because equal objects should have the same hash, this made hashing very problematic (the only sensible solution was to bin the complex amplitude via rounding). To avoid this issue, and because SymbolicPulse hashing was only used for _is_amplitude_valid, it was decided to remove the hashing option for SymbolicPulse and ScalableSymbolicPulse. Note that the caching of _is_amplitude_valid is based on a hashing which doesn't take into account the complex amp comparison.

…p,angle representation.

qiskit-bot · 2022-12-20T23:51:46Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2022-12-21T00:15:27Z

Pull Request Test Coverage Report for Build 3896155519

79 of 100 (79.0%) changed or added relevant lines in 5 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 84.616%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/library/symbolic_pulses.py	50	51	98.04%
qiskit/qpy/binary_io/schedules.py	21	41	51.22%

Files with Coverage Reduction	New Missed Lines	%
qiskit/pulse/library/symbolic_pulses.py	3	94.1%

Totals
Change from base Build 3896154873:	-0.02%
Covered Lines:	64457
Relevant Lines:	76176

💛 - Coveralls

nkanazawa1989

Overall, adding new subclass seems better than assuming a generic logic. Thanks for proposing new implementation with this PR @TsafrirA .

The implementation looks bit complicated. Current implementation seems almost identical to do

class SymbolicPulse:
    def __init__(self, ..., is_scalable=True)

without defining a subclass. To be more explicit, I think the constructor of the subclass can take amp and angle (as duration has a dedicated role), and should full-scratch own equality logic.

For QPY, I think it's better to add class name *.__class__.__name__ in the SYMBOLIC_PULSE_PACK_V2 structure, rather than adding is_scalable property. In future we may add other subclass with different set of parameter names with assigned role. So class name is much flexible (and we can avoid defining the SYMBOLIC_PULSE_PACK_V3). I'm fine with bumping the QPY version in this situation.

qiskit/pulse/library/symbolic_pulses.py

…into PulseComp3

wshanks · 2022-12-28T19:51:46Z

This implementation looks nice to me. I will let @nkanazawa1989 provide the approval.

I just want to mention a couple things about larger implications.

In future we may add other subclass with different set of parameter names with assigned role. So class name is much flexible

What kind of future pulse classes are you thinking of? If they implement features mutually exclusive to ScalabeSymbolicPulse, then subclasses make sense. If the features might overlap, then subclasses become more awkward than adding features to the base class since a pulse could only be one subclass at a time.

The other point I want to mention is that we should review when we need to check pulse equality. The main use cases that I can think of are the ones I mentioned in my inline comment about __hash__ -- when dealing with many pulse gates it can be useful to deduplicate the pulses for efficiency in processing them. Beyond that, I can imagine checking pulses for equality coming up, but I don't know that it is totally necessary (I can't think of a good case other than deduplication; maybe in round trip tests of QPY?). We might just reflect and make sure we think it is worth the extra complexity to support checking pulse equality. One of the motivations for this PR was that with the complex amp->amp+angle change in #9002 it became the case that serializing to QPY and deserializing back to Python could produce a pulse that was not equal to its original source which felt weird. Not supporting __eq__ would be an alternative solution.

…into PulseComp3

nkanazawa1989

Thanks @TsafrirA recent updates seem reasonable to me, but I also think revisiting the demand for hash is good idea. As @wshanks pointed out this is currently only used to validate amplitude (to avoid calling computationally heavy .get_waveform as much as possible).

If we don't have further motivation for hashing (I think no), we can change _is_amplitude_valid to directly call this line and let it take envelope and parameters which are both hashable.
https://github.com/Qiskit/qiskit-terra/blob/2f5944d60140ceb6e30d5b891e8ffec735247ce9/qiskit/pulse/library/symbolic_pulses.py#L490

qiskit/pulse/library/symbolic_pulses.py

qiskit/qpy/binary_io/schedules.py

qiskit/pulse/library/symbolic_pulses.py

nkanazawa1989 · 2023-01-04T16:16:58Z

What kind of future pulse classes are you thinking of? If they implement features mutually exclusive to ScalabeSymbolicPulse, then subclasses make sense. If the features might overlap, then subclasses become more awkward than adding features to the base class since a pulse could only be one subclass at a time.

I don't have any concrete idea in my mind, but I was just on the safe side. The original implementation introduced a boolean value (in the QPY data structure) to distinguish two classes. If we introduce another subclass in future PR we would need to bump the QPY version again and permanently maintain the another legacy loader. If we save class name instead of boolean we don't need to introduce such complexity.

nkanazawa1989

I like new logic for the amplitude validation. Making pulses immutable looks right approach since they can be parameterized. This is almost good to go.

qiskit/pulse/library/symbolic_pulses.py

qiskit/qpy/__init__.py

qiskit/qpy/binary_io/schedules.py

nkanazawa1989

I like new logic for the amplitude validation. Making pulses immutable looks right approach since they can be parameterized. This is almost good to go.

releasenotes/notes/new_pulse_subclass-44da774612699312.yaml

wshanks

I like the removal of __hash__ as the path forward. I noted a few small things.

qiskit/pulse/library/symbolic_pulses.py

nkanazawa1989

Thanks @TsafrirA this looks good to me.

wshanks · 2023-01-11T17:10:53Z

@nkanazawa1989 Does one of us put the automerge tag on this or do we let someone from the core team do that? I have this open in a browser tab and each time I pass by it I see it out of date with main and still re-running the checks, so I am not sure a human will ever get a chance to merge it 🙂

jakelishman · 2023-01-11T18:09:58Z

For reference: Will and I talked offline, and the answer is that code-owners are completely allowed (encouraged, even!) to apply "automerge" themselves when something's appropriate to merge. It's much better to use the bot than to do it manually because the bot will only update the PR when it's its turn to merge, avoiding unnecessary CI usage to test updates that'll never be up-to-date.

…angle representation (Qiskit#9314) * Introduced ScalableSymbolicPulses to correctly compare pulses with amp,angle representation. * Modified ScalableSymbolicPulse, bumped QPY version to 6. * Bug fix * Documentation * Release Notes * Release Notes * Release Notes * Release Notes * Release Notes * Hash correction * Resolve GaussianSquareDrag conflict. * Resolve GaussianSquareDrag conflict. * Resolve GaussianSquareDrag conflict. * GaussianSquareDrag conversion to scalable and minor fixes. * added _read_symbolic_pulse_v6, and updated _loads_operand * Added ScalableSymbolicPulse to init * Minor fixes * Documentation fix * Minor fix to amplitude validation and release notes. * Add epsilon to amp validation. Co-authored-by: Naoki Kanazawa <[email protected]> Co-authored-by: Will Shanks <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

nkanazawa1989 · 2023-01-12T01:25:56Z

Thanks Will for adding the automerge tag! I was waiting for your final review this time (if you wanted).

wshanks · 2023-01-12T18:07:21Z

I might as well mention for @nkanazawa1989 and @TsafrirA that Jake also pointed out that the reno file uses reStructuredText format rather than markdown, so for example in line code formatting should use two ` around the text instead of one.

TsafrirA · 2023-01-12T19:50:00Z

Thanks for the heads up @wshanks . This one's on me.

TsafrirA and others added 2 commits December 21, 2022 01:34

Introduced ScalableSymbolicPulses to correctly compare pulses with am…

bd2c463

…p,angle representation.

Merge branch 'Qiskit:main' into PulseComp3

ce43301

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Dec 20, 2022

nkanazawa1989 reviewed Dec 21, 2022

View reviewed changes

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

TsafrirA and others added 4 commits December 22, 2022 12:05

Modified ScalableSymbolicPulse, bumped QPY version to 6.

019057d

Merge branch 'Qiskit:main' into PulseComp3

fe6d8ad

Merge branch 'PulseComp3' of https://github.com/TsafrirA/qiskit-terra …

6f66d4e

…into PulseComp3

Bug fix

d25c1bf

TsafrirA marked this pull request as ready for review December 22, 2022 11:42

TsafrirA requested review from a team, eggerdj and wshanks as code owners December 22, 2022 11:42

TsafrirA and others added 7 commits December 22, 2022 14:15

Documentation

4d4071f

Release Notes

fcfaafe

Release Notes

56e6ffc

Release Notes

277d500

Release Notes

20e6dba

Release Notes

4bcc7c0

Merge branch 'main' into PulseComp3

d558cd4

wshanks mentioned this pull request Dec 29, 2022

Add GaussianSquareDrag symbolic pulse shape #9329

Merged

TsafrirA added 2 commits January 1, 2023 21:41

Hash correction

376dda1

Merge branch 'PulseComp3' of https://github.com/TsafrirA/qiskit-terra …

b1e4a4a

…into PulseComp3

nkanazawa1989 reviewed Jan 4, 2023

View reviewed changes

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/qpy/binary_io/schedules.py Outdated Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

ShellyGarion removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 5, 2023

ShellyGarion added this to the 0.23.0 milestone Jan 5, 2023

ShellyGarion added the mod: pulse Related to the Pulse module label Jan 5, 2023

TsafrirA mentioned this pull request Jan 8, 2023

[WIP] Fixing symbolic pulse equating for non-unique representations #9257

Closed

2 tasks

nkanazawa1989 reviewed Jan 9, 2023

View reviewed changes

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/qpy/__init__.py Outdated Show resolved Hide resolved

qiskit/qpy/binary_io/schedules.py Outdated Show resolved Hide resolved

nkanazawa1989 reviewed Jan 9, 2023

View reviewed changes

TsafrirA added 2 commits January 9, 2023 14:07

Minor fixes

466a494

Documentation fix

2f68f94

nkanazawa1989 self-assigned this Jan 9, 2023

nkanazawa1989 reviewed Jan 9, 2023

View reviewed changes

releasenotes/notes/new_pulse_subclass-44da774612699312.yaml Outdated Show resolved Hide resolved

wshanks reviewed Jan 9, 2023

View reviewed changes

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Outdated Show resolved Hide resolved

qiskit/pulse/library/symbolic_pulses.py Show resolved Hide resolved

TsafrirA added 2 commits January 10, 2023 09:08

Minor fix to amplitude validation and release notes.

35b454c

Add epsilon to amp validation.

92922bb

nkanazawa1989 approved these changes Jan 10, 2023

View reviewed changes

nkanazawa1989 and others added 2 commits January 11, 2023 02:10

Merge branch 'main' into PulseComp3

0eda2d3

Merge branch 'main' into PulseComp3

a55f4aa

wshanks approved these changes Jan 11, 2023

View reviewed changes

wshanks added 2 commits January 11, 2023 10:09

Merge branch 'main' into PulseComp3

0ca5fa1

Merge branch 'main' into PulseComp3

f671615

wshanks added Changelog: New Feature Include in the "Added" section of the changelog automerge Changelog: API Change Include in the "Changed" section of the changelog labels Jan 11, 2023

Merge branch 'main' into PulseComp3

12ae2ad

mergify bot merged commit fef0600 into Qiskit:main Jan 11, 2023

This was referenced Feb 3, 2023

Update qpy terra to 0.23.1 version Qiskit/qiskit-ibm-provider#496

Merged

Update qpy version to terra 0.23.1 Qiskit/qiskit-ibm-runtime#577

Merged

TsafrirA deleted the PulseComp3 branch June 25, 2023 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314

Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314

TsafrirA commented Dec 20, 2022 •

edited

Loading

qiskit-bot commented Dec 20, 2022

coveralls commented Dec 21, 2022 •

edited

Loading

nkanazawa1989 left a comment

wshanks commented Dec 28, 2022

nkanazawa1989 left a comment

nkanazawa1989 commented Jan 4, 2023 •

edited

Loading

nkanazawa1989 left a comment

nkanazawa1989 left a comment

wshanks left a comment

nkanazawa1989 left a comment

wshanks commented Jan 11, 2023

jakelishman commented Jan 11, 2023

nkanazawa1989 commented Jan 12, 2023

wshanks commented Jan 12, 2023

TsafrirA commented Jan 12, 2023

Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314

Introducing ScalableSymbolicPulse to correctly compare pulses of amp,angle representation #9314

Conversation

TsafrirA commented Dec 20, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Dec 20, 2022

coveralls commented Dec 21, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3896155519

💛 - Coveralls

nkanazawa1989 left a comment

Choose a reason for hiding this comment

wshanks commented Dec 28, 2022

nkanazawa1989 left a comment

Choose a reason for hiding this comment

nkanazawa1989 commented Jan 4, 2023 • edited Loading

nkanazawa1989 left a comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

wshanks left a comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

wshanks commented Jan 11, 2023

jakelishman commented Jan 11, 2023

nkanazawa1989 commented Jan 12, 2023

wshanks commented Jan 12, 2023

TsafrirA commented Jan 12, 2023

TsafrirA commented Dec 20, 2022 •

edited

Loading

coveralls commented Dec 21, 2022 •

edited

Loading

nkanazawa1989 commented Jan 4, 2023 •

edited

Loading